Repository navigation
fix(batch): release references to a rejected object in server-side batching - #2182
joaquinhuigomez wants to merge 1 commit into
Conversation
When the server rejects an object in a `stream()` batch, the error branch dropped it from the object cache but left its uuid in the lookup that holds back references until both ends are processed. Only the success branch cleared it. A reference to or from the rejected object therefore stayed queued forever: the batch loop never reached its stop sentinel, the sync `with` block took twice the shutdown timeout to exit, the reference was neither sent nor reported in `failed_references`, `flush()` never returned, and the async client raised "Background batch tasks did not terminate after forced shutdown." Clear the uuid in the error branch too, as the success branch and the client-side batching already do. The reference is then sent and its outcome comes back from the server like any other reference.
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Infrastructure as Code | View in Orca | ||
| SAST | View in Orca | ||
| Secrets | View in Orca | ||
| Vulnerabilities | View in Orca |
|
To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge. |
chrikrah
left a comment
There was a problem hiding this comment.
@joaquinhuigomez nothing blocks this. The error path now clears __uuid_lookup like the success path at async_.py:454-457 and sync.py:403-406. It uses the same lock order, so no new lock interaction.
Both new mock tests fail without the fix and pass with it:
$ pytest mock_tests/test_batch_stream_references.py -q # Python 3.12.3
2 passed, 3 warnings in 4.99s # head b504d7b
2 failed, 3 warnings in 18.95s # async_.py and sync.py from 38f30c1, tests kept
On your open question I'd keep send-and-report. Client-side batching already drops failed uuids from its lookup at base.py:736-738. It then sends their references. Failing them here as ErrorReference would split the two modes.
The CLA bot is still waiting on your agreement comment here. @g-despot, the Main workflow shows action_required, so the test CI has not run. Could you approve it?
In server-side batching, a reference whose source or target object was rejected by the server is never sent, and
batch.stream()doesn't exit cleanly either.When the server reports an object error, the results handler pops the object from
__objs_cachebut never removes its uuid from__uuid_lookup— only the success branch does that.ReferencesBatchRequest.__pop_itemsholds back any reference whose uuids are still in the lookup, so a reference touching the failed object is held forever: the background loop never reaches its stop sentinel,_BgThreads.join(timeout)returns without raising, and the context exit takes2 × (insert + 5)seconds — about three minutes at the default insert timeout — before the reference is silently dropped.failed_referencesstays empty, aBgBatchLoopthread keeps spinning after exit, and an explicitflush()never returns. The async client raisesWeaviateBatchStreamError: Background batch tasks did not terminate after forced shutdowninstead, which points at the wrong cause. The client-side batching path already removes every non-re-added uuid from its lookup, failed ones included.Fix is four lines: discard the uuid in both error branches, mirroring the success branch (sequential locks in
sync.py, nested inasync_.py, matching each file's existing style). The reference is then released and sent, and whatever the server does with it is reported honestly — it shows up infailed_referencesif rejected — instead of being lost. Exit drops to the normal ~2 s. A uuid repeated across several error messages is safe: the second pop hitsKeyErrorandcontinues before the discard, andset.discardis idempotent.Tests:
mock_tests/test_batch_stream_references.py(new file, to stay clear of #2172's edits totest_batch.py) — a mockBatchStreamrejects objectBADand any reference touching it; sync and async cases assert the reference reaches the server,failed_objects == [BAD], the reference is infailed_references, exit under 5 s, and (sync) noBgBatchLoopthread left alive. Both fail onmainwithout hanging (sync:assert 12.01 < 5; async: the forced-shutdown error after 6 s).pytest test526 passed;mock_tests67 passed; ruff (0.14.7 and 0.9.9), flake8 and pyright clean. Merge-tested against open #2141, #2172, #2180 and #2166: no textual overlap, new tests pass on each merged tree; #2141's unsent-data check only fires when the threads are dead, so it wouldn't have caught this — the two complement each other.One thing I'd flag for review rather than assert: I haven't checked whether a live server rejects a reference to a missing object or stores it dangling; the fix guarantees the reference is sent and its outcome reported, not which outcome. An alternative would be to fail such references client-side as
ErrorReferencewithout sending them — happy to switch if that's the preferred semantics.